Ziga/add personal sign authentication message #1811

zkokelj · 2024-02-21T11:38:37Z

Why this change is needed

We want to use Rainbowkit (https://www.rainbowkit.com/) in the future and they don't support EIP-712 messages yet.
To use them we need to add personal sign message, because it is impossible to overwrite signer there.
In order to avoid confusion I also refactored and simplified parts of code for signature verification and introduced signatureType.

What changes were made as part of this PR

Added new personal sign message and methods to verify it.
Refactored code around verifying signatures
Added signature type
Refactored viewing key tests

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

PR checks reviewed and performed

go/common/viewingkey/viewing_key.go

…ple versions

tudor-malene

thanks Ziga. Much better now.
A few improvements and this should be good to merge

tudor-malene · 2024-03-14T09:25:58Z

go/common/viewingkey/viewing_key.go

+var PersonalSignMessageSupportedVersions = []int{1}
+
+// SignatureType is used to differentiate between different signature types (string is used, because int is not RLP-serializable)
+type SignatureType string


you can make SignatureType an iota to avoid the conversions and minimize the data

This was my initial approach, but having SignatureType of type int causes problems with RLP serialisation.
And errors like this:

Cause: rlp: type int is not RLP-serializable (struct field viewingkey.RPCSignedViewingKey.SignatureType) (struct field common.LogSubscription.ViewingKey)"}} component=test_log

I can change it to []byte to save some space. What do you think? Do you have some other ideas?

what about int8 or one of the other int types?

all int types fail with the same error unfortunately

what about uints?

tudor-malene · 2024-03-14T09:32:49Z

go/common/viewingkey/viewing_key.go

@@ -245,8 +274,8 @@ func CheckSignatureAndReturnAccountAddress(hashBytes []byte, signature []byte) (
 	return nil, fmt.Errorf("invalid signature")
 }

-// CheckEIP712Signature checks if signature is valid for provided userID and chainID and return address or nil if not valid
-func CheckEIP712Signature(userID string, signature []byte, chainID int64) (*gethcommon.Address, error) {
+// checkEIP712Signature checks if signature is valid for provided userID and chainID and return address or nil if not valid


it would be nice if these functions each lived in their own type behind an interface

tudor-malene · 2024-03-14T09:33:45Z

go/common/viewingkey/viewing_key.go

+func CheckSignature(encryptionToken string, signature []byte, chainID int64, signatureType SignatureType) (*gethcommon.Address, error) {
+	if signatureType == PersonalSign {
+		return checkPersonalSignSignature(encryptionToken, signature, chainID)
+	} else if signatureType == EIP712Signature {


if these checks were each in their type, then you can have a map of registered signature handlers, and make this method much simpler

tudor-malene · 2024-03-14T09:53:51Z

tools/walletextension/lib/client_lib.go

+func (o *TGLib) RegisterAccountPersonalSign(pk *ecdsa.PrivateKey, addr gethcommon.Address) error {
+	// create the registration message
+	personalSignMessage := viewingkey.GeneratePersonalSignMessage(string(o.userID), integration.TenChainID, 1)
+	prefixedMessage := fmt.Sprintf(viewingkey.PersonalSignMessagePrefix, len(personalSignMessage), personalSignMessage)


shouldn't this prefix logic be in the GeneratePersonalSignMessage function as well?

Good point. Actually this prefixing can be removed by using accounts.TextHash from geth, which adds the prefix and hashes the message + keeping it without a prefix is better for the new endpoints (wallets like Metamask automatically adds this prefix to the message)

zkokelj · 2024-03-14T16:01:16Z

General comment / question.

Is it critical if our system is vulnerable to the following (possible attack)?

When users sign the message to authenticate them they have 3 options (legacy will be removed and I won't talk about it).
personalSign and EIP712Sign. This is used to prove, that they are actually the owners of that account.
But if user sends the signature to the node and sets the message type to the other type the node "thinks" that it is another address and does not detect that. To be able to detect that we would need to include messageHash with the signature.
What do you think @tudor-malene ?

tudor-malene · 2024-03-15T09:48:12Z

General comment / question.

Is it critical if our system is vulnerable to the following (possible attack)?

When users sign the message to authenticate them they have 3 options (legacy will be removed and I won't talk about it). personalSign and EIP712Sign. This is used to prove, that they are actually the owners of that account. But if user sends the signature to the node and sets the message type to the other type the node "thinks" that it is another address and does not detect that. To be able to detect that we would need to include messageHash with the signature. What do you think @tudor-malene ?

If I understand the attack correctly, the user wants to read the data of an account they don't control. For this, they have to produce a signed message of one of the 2 formats that contains the target address in the right place and sign that with a key they control that somehow, when going through our recovery algorithm will produce the right values?

tudor-malene

thanks Ziga. I think it looks good now. The responsibilities are quite clear.
One last thing to check if other int data types work for the type .

tudor-malene · 2024-03-15T09:49:34Z

go/common/viewingkey/viewing_key.go

+var PersonalSignMessageSupportedVersions = []int{1}
+
+// SignatureType is used to differentiate between different signature types (string is used, because int is not RLP-serializable)
+type SignatureType string


what about int8 or one of the other int types?

tudor-malene · 2024-03-15T09:50:20Z

go/common/viewingkey/viewing_key.go

+	SignatureType           SignatureType
+}
+
+// SignatureChecker is an interface for checking if signature is valid for provided encryptionToken and chainID and return address or nil if not valid


return "signing address"

tudor-malene

lgtm

zkokelj marked this pull request as ready for review February 22, 2024 11:55

zkokelj force-pushed the ziga/add_personal_sign_authentication_message branch from 7c02e15 to 1976388 Compare February 22, 2024 15:13

tudor-malene requested changes Feb 26, 2024

View reviewed changes

go/common/viewingkey/viewing_key.go Outdated Show resolved Hide resolved

zkokelj force-pushed the ziga/add_personal_sign_authentication_message branch 2 times, most recently from 47cbbb3 to 2c1f41c Compare March 5, 2024 10:23

zkokelj added 13 commits March 13, 2024 14:31

add personal sign format

f283a79

fix

8ecd521

lint

8c2d49f

add type for gateway check

285f9a6

store signature type in the database

cbb2317

include tests

c9a3d3d

lint

b447404

fix db account

4f688a0

signature type added

174fee8

remove account

38418ac

simplify legacy format checks

8350299

removed second EIP-712 version option, but keep possibility for multi…

682e698

…ple versions

fix tests

c2f85ed

zkokelj force-pushed the ziga/add_personal_sign_authentication_message branch from f148e94 to c2f85ed Compare March 13, 2024 14:37

tudor-malene reviewed Mar 14, 2024

View reviewed changes

zkokelj added 2 commits March 14, 2024 16:08

use accounts.TextHash instead of manually adding a prefix

4cced4a

PR fixes

a654694

zkokelj added 3 commits March 14, 2024 19:30

add legacy signature checker

1973905

lint

24d6254

fix legacy signature checking

1886d78

tudor-malene reviewed Mar 15, 2024

View reviewed changes

zkokelj added 2 commits March 15, 2024 12:55

fixed comment

2cbf196

use uint8 as SignatureType

ebfed3e

fix

18ed245

tudor-malene approved these changes Mar 18, 2024

View reviewed changes

zkokelj merged commit a38b4ab into main Mar 18, 2024
2 checks passed

zkokelj deleted the ziga/add_personal_sign_authentication_message branch March 18, 2024 11:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ziga/add personal sign authentication message #1811

Ziga/add personal sign authentication message #1811

zkokelj commented Feb 21, 2024 •

edited

Loading

tudor-malene left a comment

tudor-malene Mar 14, 2024

zkokelj Mar 14, 2024 •

edited

Loading

tudor-malene Mar 15, 2024

zkokelj Mar 15, 2024

tudor-malene Mar 15, 2024

tudor-malene Mar 14, 2024

zkokelj Mar 14, 2024

tudor-malene Mar 14, 2024

zkokelj Mar 14, 2024

tudor-malene Mar 14, 2024

zkokelj Mar 14, 2024

zkokelj commented Mar 14, 2024

tudor-malene commented Mar 15, 2024

tudor-malene left a comment

tudor-malene Mar 15, 2024

tudor-malene Mar 15, 2024

tudor-malene left a comment

Ziga/add personal sign authentication message #1811

Ziga/add personal sign authentication message #1811

Conversation

zkokelj commented Feb 21, 2024 • edited Loading

Why this change is needed

What changes were made as part of this PR

PR checks pre-merging

tudor-malene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkokelj Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkokelj commented Mar 14, 2024

tudor-malene commented Mar 15, 2024

tudor-malene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tudor-malene left a comment

Choose a reason for hiding this comment

zkokelj commented Feb 21, 2024 •

edited

Loading

zkokelj Mar 14, 2024 •

edited

Loading